Skip to content

Concatenation of series of differing types should lead to object #21922

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

xhochy
Copy link
Contributor

@xhochy xhochy commented Jul 15, 2018

I see no simple solution on how to make a test for this without introducing a whole new ExtensionArray class that is actually of scalar type 'i'.

cc @jreback I'm wondering a bit why this has not yet been a problem for #21160

I have check that this fixes the problems I'm seeing in fletcher on int and float dtypes.

@jreback
Copy link
Contributor

jreback commented Jul 16, 2018

this is not generally the correct solution, the EA should decide what the result should be. we need additional API for this I think in EA.

@jreback
Copy link
Contributor

jreback commented Jul 16, 2018

best also to do an example with the test EA class

@gfyoung gfyoung added Bug ExtensionArray Extending pandas with custom dtypes or arrays. labels Jul 16, 2018
@jorisvandenbossche
Copy link
Member

this is not generally the correct solution, the EA should decide what the result should be. we need additional API for this I think in EA.

As long as we do not have such additional api that handles negotation between different EA types on what the result should be, I think this PR is the good solution.

best also to do an example with the test EA class

@xhochy mentioned there is no test EA class that can do this (eg with the DecimalArray test case you don't get this bug)
How much code would it be to have a minimal EA test class to test this bug?

@jreback
Copy link
Contributor

jreback commented Jul 20, 2018

As long as we do not have such additional api that handles negotation between different EA types on what the result should be, I think this PR is the good solution.

and this PR should propose such a solution. otherwise everything is just a band aid.

@jorisvandenbossche
Copy link
Member

without introducing a whole new ExtensionArray class that is actually of scalar type 'i'.

The question is maybe also in general if it is needed to have a 'i' kind in your fletcher dtypes (instead of always 'O'). I am not sure to what extent we currently use that information for EAs.

@jorisvandenbossche
Copy link
Member

and this PR should propose such a solution. otherwise everything is just a band aid.

Why? We already do exactly the same for dataframes, there we simply always convert to object if they are not matching dtypes.
I think it is good to already makes this uniform with Series, regardless of whether / when we introduce such a more general solution.

@jreback
Copy link
Contributor

jreback commented Jul 20, 2018

Why? We already do exactly the same for dataframes, there we simply always convert to object if they are not matching dtypes.

not sure what you are talking about, this is the obvious case of having an EA be able to do something intelligent on concat rather than simply coercing. It can always raise not implemented error and then this soln is fine.

In any event this needs tests.

@jorisvandenbossche
Copy link
Member

not sure what you are talking about

Did you see the example in the issue? () Concatting a dataframe already works (if converting to object is assumed to be the correct behaviour, which it is at the moment IMO), concatting a series not. This PR is fixing that inconsistency.

@jorisvandenbossche
Copy link
Member

As I said before, I think this is a reasonable fix. @jreback can respond again (also to my last comment, this is basically doing for Series what already is done for DataFrame. The main difficulty is tests)

@jreback
Copy link
Contributor

jreback commented Nov 21, 2018

I am still not clear what this is fixing, this would need a general extension test.

@jreback
Copy link
Contributor

jreback commented Dec 23, 2018

closing. this needs tests, if you want to continue pls ping.

@jreback jreback closed this Dec 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug ExtensionArray Extending pandas with custom dtypes or arrays.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pd.concat of Series with int64 column and Series with int64-ExtensionArray yields int64
4 participants